feat(admin): add CSRF protection to mutation endpoints#83
Merged
Conversation
Closes #79. Admin POST/PUT/DELETE routes now require a server-validated CSRF token in addition to the session cookie. Token is per-session, minted lazily when the dashboard renders, rotated on OAuth login, and validated via a FastAPI dependency on each mutation route. Clients send it in the X-CSRF-Token header (HTMX listener reads the token from a <meta> tag) with a csrf_token form-field fallback. Dev-skip-auth mode bypasses CSRF to match auth bypass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds CSRF protection for admin state-changing endpoints by minting a per-session CSRF token, validating it server-side on mutation routes, and propagating it to the client (via a <meta name="csrf-token"> tag + an HTMX htmx:configRequest listener).
Changes:
- Introduces
services/csrf.pywith CSRF token minting (get_or_create_csrf_token) and validation (verify_csrf_token) logic. - Applies CSRF validation as a FastAPI dependency on admin mutation routes and threads the token into the admin template render context.
- Updates bundled admin themes to emit the CSRF meta tag and attach
X-CSRF-Tokento HTMX requests; adds unit tests for CSRF behaviors.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/squishmark/services/csrf.py | Adds CSRF token generation + verification dependency used by admin mutation endpoints. |
| src/squishmark/routers/admin.py | Mints token during admin dashboard render and enforces CSRF validation on POST/PUT/DELETE routes. |
| src/squishmark/routers/auth.py | Rotates CSRF token on successful OAuth login by clearing the session CSRF entry. |
| themes/default/admin/admin.html | Renders CSRF token meta tag and adds HTMX request hook to send X-CSRF-Token. |
| themes/blue-tech/admin/admin.html | Renders CSRF token meta tag and adds HTMX request hook to send X-CSRF-Token. |
| themes/terminal/admin/admin.html | Renders CSRF token meta tag for the terminal theme’s admin UI. |
| themes/terminal/static/js/admin.js | Adds HTMX request hook to send X-CSRF-Token for the terminal theme. |
| tests/test_csrf.py | Adds unit tests for token minting, validation paths, and dev-skip bypass behavior. |
…EY constant Address Copilot review on PR #83: 1. auth.py now uses SESSION_KEY constant from services.csrf instead of hardcoding 'csrf_token' — avoids key drift if the storage key ever changes. 2. New GET /admin/csrf endpoint returns the current token in JSON, matching the issue's acceptance criterion for non-HTML clients. The module docstring already promised JSON API support; this makes good on it. Tests added for the new endpoint (idempotent within session) and the auth rotation behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address Copilot review pass 2 on PR #83: Route-level dependencies=[Depends(verify_csrf_token)] resolves before parameter deps (including AdminUser), so unauthenticated HTMX requests would get 403 (CSRF token missing) instead of the intended 401 + HX-Redirect. Fix by: 1. Move get_current_admin from routers/admin.py to dependencies.py so services/csrf.py can import it without a circular dep. Re-exported from admin.py so existing test patches keep working. 2. verify_csrf_token now depends on get_current_admin via Annotated[str, Depends(...)]. FastAPI resolves auth first; if it raises 401, CSRF never runs. Also rewrote test_oauth_callback_rotates_csrf_token to actually exercise oauth_callback with mocked httpx calls (per Copilot feedback — the previous version only tested dict.pop). Added a structural test that locks in the new CSRF→auth dep ordering. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address Copilot review pass 3 on PR #83: 1. Centralize is_htmx in dependencies.py and import it from routers/admin.py. Previously the prior commit duplicated the helper (as _is_htmx in dependencies.py alongside the existing is_htmx in admin.py). 2. Update test patches that target squishmark.routers.admin.get_settings to squishmark.dependencies.get_settings. After moving get_current_admin to dependencies.py the old patches were silently no-ops — tests passed only because the real settings happened to produce the same outcome for an empty session. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…polish Independent code review pass on PR #83. Six changes: 1. tests/test_csrf_integration.py (new): five TestClient-based tests that actually exercise the route-level CSRF dep and the auth-before-CSRF ordering — previously all four 'dependencies=[Depends(verify_csrf_token)]' lines were uncovered (existing tests call handler functions directly and bypass FastAPI's DI). Includes the crucial 'unauth HTMX → 401+HX-Redirect, not 403' assertion. 2. services/csrf.py: collapse the two distinct error messages ('CSRF token missing from session' vs 'CSRF token invalid') into one generic 'CSRF validation failed'. The prior split was an information disclosure — it told an attacker whether they had a session cookie to bind to. 3. services/csrf.py: document the assumption that Starlette caches request.form() so the dep + route handler can both read it without losing the body. 4. routers/admin.py: drop the __all__ re-export of get_current_admin / AdminUser. Test imports updated to point at squishmark.dependencies directly — there were no production callers. 5. routers/admin.py: GET /admin/csrf now returns a typed CSRFTokenResponse Pydantic model instead of a raw dict, matching the convention of NoteResponse and CacheRefreshResponse. 6. tests/test_admin_notes.py: oauth_callback rotation test now also asserts a fresh token can be minted after rotation, not just that the stale one is gone. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two structured logs added so production issues are diagnosable:
1. verify_csrf_token now emits WARNING on every rejection with reason (no-session-token / no-submitted-token / token-mismatch), method, path, admin, and htmx flag. The user-facing detail stays generic ('CSRF validation failed') to avoid info disclosure — the log is the only place the specific failure mode lives. Caplog test locks the format in.
2. oauth_callback emits INFO on CSRF token rotation, tying the rotation to the user that just logged in. Useful for audit trails and login-flow debugging.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #79.
Summary
dependencies=[Depends(verify_csrf_token)])<meta name="csrf-token">tag and attaches it asX-CSRF-Tokento every request;csrf_tokenform-field fallback also accepteddev_skip_authmode bypasses CSRF to match the existing auth-bypass behaviorChanges
services/csrf.py—get_or_create_csrf_token+verify_csrf_token(usessecrets.token_urlsafe(32)andsecrets.compare_digest)routers/admin.py— token threaded into dashboard render context;verify_csrf_tokendep oncreate_note,update_note,delete_note,refresh_cacherouters/auth.py— popscsrf_tokenfrom session on successful OAuth callback so a fresh token mints on next readthemes/{default,blue-tech,terminal}/admin/admin.html— meta tag injected in<head>; HTMXconfigRequestlistener added (inline in default/blue-tech; inthemes/terminal/static/js/admin.jsfor terminal)tests/test_csrf.py— 11 new unit tests covering mint/idempotency, header path, form fallback, missing-token/wrong-token rejection, dev-skip bypass, prod-mode enforcementTest plan
python scripts/run-checks.py— 221 tests pass, ruff clean, 0 pyright errorsX-CSRF-Token, mutations succeeddev_skip_auth=true: dashboard loads, note CRUD via HTMX works🤖 Generated with Claude Code